Skip to content

feat(world-builder): register uploaded graphics and extend palette schemas (#6) - #130

Open
angelTomo9 wants to merge 7 commits into
Bitcoindefi:mainfrom
angelTomo9:feat-world-builder-graphics-palette-1787657711522
Open

feat(world-builder): register uploaded graphics and extend palette schemas (#6)#130
angelTomo9 wants to merge 7 commits into
Bitcoindefi:mainfrom
angelTomo9:feat-world-builder-graphics-palette-1787657711522

Conversation

@angelTomo9

Copy link
Copy Markdown

Closes #6

Summary of Changes

Bridges uploaded custom PNG assets with the game engine runtime, allowing uploaded graphics to be resolved by client renderers and assigned into dynamic map palettes.

Features & Deliverables

  • Reserved Non-Colliding ID Range: Uploaded graphics use dedicated index space >= 1,000,000, preventing collision with legacy original engine graphics (1..320,151).
  • Graphic Metadata Resolution Endpoint: Added GET /graphics/:grhIndex/metadata returning frame count, dimension, offset, and URL metadata for engine compatibility.
  • Dynamic Palette Overrides: Added game_map_palette_overrides table in api/schema.sql and endpoints PUT /admin/game-data/maps/:mapNum/palette & GET /admin/game-data/maps/:mapNum/palette.
  • Defensive Validation: Strict validation rejects references to unuploaded or out-of-range graphic indexes before they can corrupt tile rendering.
  • Integration Tests: Added unit & integration test coverage in api/src/tests/worldBuilder_palette.integration.test.ts.

Comment thread api/src/server.ts
Comment thread api/src/repositories/worldBuilder.ts Outdated
@gitar-bot

gitar-bot Bot commented Aug 25, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 2 resolved / 2 findings

Registers uploaded custom PNG assets and extends palette schemas for the world builder, addressing missing authentication on the palette endpoint and race conditions in upsertPaletteEntry.

✅ 2 resolved
Security: GET /admin/game-data/maps/:mapNum/palette lacks authentication

📄 api/src/server.ts:962-976
Unlike every sibling admin map endpoint (PUT palette, tiles, publish, discard, revert, permissions), this GET handler never calls requireMapEditSession or requireAdminEmailSession, and there is no global admin auth middleware (confirmed: app.use only does JSON parsing, logging, CORS). Any unauthenticated client can enumerate a map's dynamic palette overrides, including uploaded graphic index assignments. Add an auth guard, e.g. call const authorized = await requireMapEditSession(request, response, mapNum); if (!authorized) return; (or requireAdminEmailSession) before listMapPalette.

Bug: Concurrent upsertPaletteEntry can overwrite existing palette rows

📄 api/src/repositories/worldBuilder.ts:619-633
When entry.paletteId is omitted, the next id is derived from SELECT MAX(palette_id)+1. Two concurrent requests for the same mapNum compute the same next_id; the first INSERT succeeds and the second hits ON CONFLICT (map_num, palette_id) DO UPDATE, silently overwriting the just-created entry instead of allocating a new id. Allocate the id atomically (e.g. a per-map sequence, a SERIAL/IDENTITY column, or wrapping the select+insert in a transaction with row locking) so concurrent new entries don't clobber each other.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Important

Your trial ends in 7 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.

Was this helpful? React with 👍 / 👎 | Gitar

@leocagli

Copy link
Copy Markdown
Collaborator

Dejo esta abierta de las cuatro (#130, #131, #132, #133), porque es la unica cuyo titulo describe el bulto real del diff: las ~550 lineas de worldBuilder.ts mas api/src/server.ts y los dos tests de world-builder, que las cuatro traian igual. La medicion completa quedo escrita en las que cerre.

Dos cosas antes de retomarla:

Quedo en conflicto. Se mergeo #109 (Stage 4 del editor), que reescribio worldBuilder.ts y api/src/server.ts justo en esa zona. Hay que rebasar contra main y revisar que de esto sigue haciendo falta: parte puede haber quedado cubierta.

Lo que no es world-builder va aparte. El apagado ordenado, la penalizacion por sesion doble y las variables de SES son tres cambios distintos que viajaban de polizon en este diff. Cada uno merece su PR, con su diff y nada mas. El apagado ordenado ya lo tenes asi en #48, que esta limpia y mergeable: ese es el formato que conviene.

main esta en verde desde hoy, asi que el CI ya sirve como senal de verdad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Etapa 1: registrar PNG subidos como graficos del motor y extender la paleta

2 participants